Reorganize ext/uri tests - withers #19970

kocsismate · 2025-09-26T06:44:20Z

No description provided.

ext/uri/tests/whatwg/modification/host_error_forbidden_host_codepoint_opaque2.phpt

kocsismate · 2025-09-26T06:51:37Z

ext/uri/tests/whatwg/modification/path_success_auto_encoded.phpt

The ^ character is in the percent-encode set of the path (https://url.spec.whatwg.org/#path-percent-encode-set), but somehow it's not percent-encoded.

TimWolla

I went through all RFC 3986 tests. Looking pretty good overall, some nits. I also think it would be helpful to print the entire resulting URL in addition to printing the output of the getters, this makes it easier to double-check how exactly the resulting URL will look, particularly with regard to the difference between null and "" (e.g. whether there is a ? for the query or not).

ext/uri/tests/rfc3986/modification/fragment_success_encoded.phpt

TimWolla · 2025-09-27T18:20:11Z

ext/uri/tests/rfc3986/modification/fragment_success_encoded.phpt

+<?php
+
+$uri1 = Uri\Rfc3986\Uri::parse("https://example.com");
+$uri2 = $uri1->withFragment("foo%3Dbar"); // foo=bar


Should this also have a normalizable percent-encoded character similarly to ext/uri/tests/rfc3986/modification/host_success_encoded.phpt?

This seems to be unresolved still.

ext/uri/tests/rfc3986/modification/host_success_ipv6.phpt

ext/uri/tests/rfc3986/modification/path_error_reserved.phpt

ext/uri/tests/rfc3986/modification/port_error_overflow.phpt

ext/uri/tests/rfc3986/modification/roundtrip_special_case3.phpt

ext/uri/tests/rfc3986/modification/scheme_error_empty.phpt

ext/uri/tests/rfc3986/modification/scheme_error_reserved.phpt

ext/uri/tests/rfc3986/modification/userinfo_success_existing.phpt

ext/uri/tests/rfc3986/modification/userinfo_success_new.phpt

kocsismate · 2025-09-28T12:51:55Z

I also think it would be helpful to print the entire resulting URL in addition to printing the output of the getters, this makes it easier to double-check how exactly the resulting URL will look, particularly with regard to the difference between null and "" (e.g. whether there is a ? for the query or not).

Yes, I was also thinking about something similar (at least in some edge cases), but I didn't want to make the output too "crowded". My other concern is that most states can be tested via regular parsing 🤔 (we don't necessary have to use withers for them)

kocsismate · 2025-09-29T19:54:18Z

@TimWolla What do you think about verifying URI recomposition separately? Or do you think we should always test the recomposed URI after URI modification?

TimWolla · 2025-10-04T09:46:04Z

Or do you think we should always test the recomposed URI after URI modification?

Always, because this also helps the human reader understand the test better.

TimWolla · 2025-10-04T09:47:15Z

but I didn't want to make the output too "crowded".

If the tests follow a common structure / order, that's easy to check and scan. You could also insert an additional newline between the various output “sections”.

nielsdos · 2025-10-04T23:07:06Z

Unless I'm really needed here I'm removing my review request.
The honest reason is that I'm not really looking forward to reviewing 2K lines of tests in my spare time 😅

TimWolla

Taken another look at the RFC 3986 tests now. Generally LGTM. Will still need to look at the WHATWG tests.

ext/uri/tests/rfc3986/modification/host_success_encoded.phpt

TimWolla · 2025-10-05T13:02:24Z

ext/uri/tests/rfc3986/modification/fragment_success_encoded.phpt

+<?php
+
+$uri1 = Uri\Rfc3986\Uri::parse("https://example.com");
+$uri2 = $uri1->withFragment("foo%3Dbar"); // foo=bar


This seems to be unresolved still.

ext/uri/tests/rfc3986/modification/fragment_success_encoded.phpt

ext/uri/tests/rfc3986/modification/path_success_encoded.phpt

ext/uri/tests/rfc3986/modification/query_success_context_sensitive_reserved.phpt

ext/uri/tests/rfc3986/modification/query_success_encoded.phpt

ext/uri/tests/rfc3986/modification/scheme_success_basic.phpt

TimWolla · 2025-10-12T20:12:16Z

ext/uri/tests/whatwg/modification/fragment_success_empty.phpt

Can you also add a test with ->withFragment("#")?

TimWolla · 2025-10-12T20:13:21Z

ext/uri/tests/whatwg/modification/host_error_forbidden_host_codepoint_opaque2.phpt

Should we have this test also for a special scheme for symmetry between the opaque and special tests?

TimWolla · 2025-10-12T20:14:29Z

ext/uri/tests/whatwg/modification/password_success_existing.phpt

@@ -0,0 +1,19 @@
+--TEST--
+Test Uri\WhatWg\Url component modification - password - changing an existing one


I think the description and filename doesn't match what the test does. It's a duplicate of ext/uri/tests/whatwg/modification/password_success_new.phpt.

TimWolla · 2025-10-12T20:15:24Z

ext/uri/tests/whatwg/modification/password_success_unset_existing.phpt

@@ -0,0 +1,19 @@
+--TEST--
+Test Uri\WhatWg\Url component modification - username - unsetting existing


This description does not match the test and does not match the filename.

TimWolla · 2025-10-12T20:15:27Z

ext/uri/tests/whatwg/modification/password_success_unset_non_existent.phpt

@@ -0,0 +1,19 @@
+--TEST--
+Test Uri\WhatWg\Url component modification - username - unsetting non-existent


This description does not match the test and does not match the filename.

TimWolla · 2025-10-12T20:16:42Z

ext/uri/tests/whatwg/modification/port_error_overflow.phpt

Can be removed, this is just testing the signature (similarly to the one in Rfc3986 that was already removed).

TimWolla · 2025-10-12T20:17:44Z

ext/uri/tests/whatwg/modification/query_success_context_senstivie_reserved.phpt

Typo in filename

TimWolla · 2025-10-12T20:18:03Z

ext/uri/tests/whatwg/modification/query_success_empty.phpt

Similarly: Please add a test with "?".

TimWolla · 2025-10-12T20:19:21Z

ext/uri/tests/whatwg/modification/username_success_existing.phpt

Duplicate of ext/uri/tests/whatwg/modification/username_success_new.phpt. Test doesn't match the description / filename.

TimWolla · 2025-10-12T20:19:55Z

ext/uri/tests/whatwg/modification/username_success_unset_non_existent.phpt

+--FILE--
+<?php
+
+$url1 = Uri\WhatWg\Url::parse("https://example.com");


Suggested change

$url1 = Uri\WhatWg\Url::parse("https://example.com");

$url1 = Uri\WhatWg\Url::parse("https://:password@example.com");

Should we also test this?

kocsismate requested review from TimWolla and nielsdos September 26, 2025 06:44

github-actions bot added the Extension: uri label Sep 26, 2025

kocsismate commented Sep 26, 2025

View reviewed changes

kocsismate force-pushed the ext-uri-tests1 branch from 00e380e to dd5e549 Compare September 26, 2025 06:52

kocsismate changed the base branch from master to PHP-8.5 September 26, 2025 07:40

Reorganize ext/uri tests - withers

059b1a1

kocsismate force-pushed the ext-uri-tests1 branch from dd5e549 to 059b1a1 Compare September 27, 2025 11:31

TimWolla reviewed Sep 27, 2025

View reviewed changes

Address review comments

d32421f

kocsismate added 2 commits October 4, 2025 15:38

Display full URI

26e17e4

Remove unnecessary test

98f5282

nielsdos removed their request for review October 4, 2025 23:06

TimWolla reviewed Oct 5, 2025

View reviewed changes

Address review comments

48072a6

TimWolla reviewed Oct 12, 2025

View reviewed changes

		@@ -0,0 +1,19 @@
		--TEST--
		Test Uri\WhatWg\Url component modification - password - changing an existing one

	$url1 = Uri\WhatWg\Url::parse("https://example.com");
	$url1 = Uri\WhatWg\Url::parse("https://:password@example.com");

Reorganize ext/uri tests - withers #19970

Are you sure you want to change the base?

Reorganize ext/uri tests - withers #19970

Conversation

kocsismate commented Sep 26, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimWolla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kocsismate commented Sep 28, 2025

Uh oh!

kocsismate commented Sep 29, 2025

Uh oh!

TimWolla commented Oct 4, 2025

Uh oh!

TimWolla commented Oct 4, 2025

Uh oh!

nielsdos commented Oct 4, 2025

Uh oh!

TimWolla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants